Skip to content

Refactored code#106

Closed
dcrec1 wants to merge 17 commits intorails:masterfrom
dcrec1:master
Closed

Refactored code#106
dcrec1 wants to merge 17 commits intorails:masterfrom
dcrec1:master

Conversation

@dcrec1
Copy link

@dcrec1 dcrec1 commented Feb 20, 2011

Hi, I noticed the code had some duplications and some conditionals that could be avoided and decided to refactor it.

The only thing that changed in the behavior is the modification of 'return false' to 'event.preventDefault()'. I couldn't test this because the tests were being included before rails.js, so I couldn't bind an event to an element after the ujs events to test if they were being called, but I can try to find a way.

@mislav
Copy link
Member

mislav commented Feb 21, 2011

There are good parts, there are bad (meaning: unwanted) parts.

Parts that I like: attr('data-') changed to data(), val() invocations with a function (didn't know about that), splitting of handleRemote function. Switching to preventDefault instead of return false is OK also, I guess, that lets the event bubble up so user code can know it happened. This last thing, definitely make sure you do it in its own commit (because it's a feature change).

Parts that I don't like: indentation changes (indentation is currently one tab; I might change it to 2 spaces later on), extending $.fn (rails.js is not a jQuery plugin, it doesn't publish any public APIs), and naming of more than a few methods, mostly ones you use near the end of your changes (related to your $.fn extensions).

So, what I want you to do is redo (or rebase) your changes, grouping them one change at a time (try not to have "dryed some code" commits), starting with changes I liked, avoiding changing indentation, then you can end with changes I didn't like ($.fn extensions and non-descriptive function names) so I can review them again and think about how I can improve on what you already did.

Thanks for taking time to improve this code. Sorry if you think I'm being overly critical—I'm very sensitive about code style, naming, and grouping changes in a logical way (I sometimes spend even up to several hours rebasing my own feature branches in larger projects)

@dcrec1
Copy link
Author

dcrec1 commented Feb 21, 2011

Sorry for the indentation changes, I'll adjust them to follow the JS standard.

About the jQuery prototype extensions, I thought the code was more readable like this but I can convert this calls to local functions if you think the extensions can cause any problem.

About the method names, are you talking about register and isMissing? If yes, I think that by themselves they're not too expressive but I think they fit well in the code:

.filter(isMissing).
.ifAllowedOn('click', register);

Anyway, just please confirm me what you didn't like so I can fix them.

About the commits, I tried already to commit one change at time. Can you please told me which one(s) do you thought it's too big so I can improve it?

Thanks for the feedback and don't feel sorry, I'm exactly like you :)

@neerajsingh0101
Copy link

I like some of individual commits but the pull request as a whole has a lot of changes. I would suggest lets move slowly.

Commit like dcrec1@ce01aad should be merged into some other commit.

Anyway let's start over. How about sending a pull request with just these two commits:

dcrec1@4ce77d8

dcrec1@0748cac

Hopefully I am not being too critical. I am just trying to ensure that all your hard work gets merged. However in order to get code merged it is easier to look at individual pieces.

Let me know if you have any questions.

@neerajsingh0101
Copy link

I am closing this as part of clean up.

@diego if you upto then please make the changes incrementally and please send different pull requests for different thing. Thanks

@dcrec1
Copy link
Author

dcrec1 commented Apr 13, 2011

Ok Neeraj, will pick some time to do it, thanks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants